Skip to content

Comments

feat(ctb): simplify XDM initialization#3443

Merged
mergify[bot] merged 1 commit intodevelopfrom
sc/ctb-simplify-init
Sep 16, 2022
Merged

feat(ctb): simplify XDM initialization#3443
mergify[bot] merged 1 commit intodevelopfrom
sc/ctb-simplify-init

Conversation

@smartcontracts
Copy link
Contributor

Description
Simplifies XDM initialization by moving all input variables into the constructor. This was the best I could do, we still need an initializer because of the stuff that the base contract inherits. The primary benefit of this change is that it means we never have to put any parameters into the initializer, which means we can be very certain about parameters before we perform the upgrade.

Tests
Core functionality is not changing and tests continue to pass.

@changeset-bot
Copy link

changeset-bot bot commented Sep 13, 2022

🦋 Changeset detected

Latest commit: 8f7d008

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages
Name Type
@eth-optimism/contracts-bedrock Patch
@eth-optimism/actor-tests Patch
@eth-optimism/sdk Patch
@eth-optimism/drippie-mon Patch
@eth-optimism/message-relayer Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added 2-reviewers A-pkg-contracts-bedrock Area: packages/contracts-bedrock labels Sep 13, 2022
@tynes
Copy link
Contributor

tynes commented Sep 13, 2022

I think this will also need changes in the go L1 genesis creation code

@smartcontracts smartcontracts requested review from maurelian and tynes and removed request for tynes September 15, 2022 15:27
@mergify
Copy link
Contributor

mergify bot commented Sep 15, 2022

Hey @smartcontracts! This PR has merge conflicts. Please fix them before continuing review.

@mergify mergify bot added the conflict label Sep 15, 2022
@mergify mergify bot removed the conflict label Sep 15, 2022
@smartcontracts smartcontracts force-pushed the sc/ctb-simplify-init branch 2 times, most recently from 9e0e7f9 to c208791 Compare September 15, 2022 17:37
@tynes
Copy link
Contributor

tynes commented Sep 15, 2022

Encountered 1 failing test in contracts/test/OptimismPortal.t.sol:OptimismPortal_FinalizeWithdrawal_Test [FAIL. Reason: EvmError: Revert Counterexample: calldata=0x2147ae39000000000000000000000000b4c79dab8f259c7aee6e5b2aa729821864227e8400000000000000000000000000000000000000000000000000000000000080c500000000000000000000000000000000000000000000000000000000000004cc00000000000000000000000000000000000000000000000000000000000001ca00000000000000000000000000000000000000000000000000000000000000a0000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000bcfceff2, args=[0xb4c79dab8f259c7aee6e5b2aa729821864227e84, 0x00000000000000000000000000000000000080c5, 1228, 458, 0x00000000000000000000000000000000000000000000000000000000bcfceff2]] test_finalizeWithdrawalTransaction_differential(address,address,uint256,uint256,bytes) (runs: 29, μ: 264541, ~: 265729) 

🤔

@smartcontracts
Copy link
Contributor Author

Encountered 1 failing test in contracts/test/OptimismPortal.t.sol:OptimismPortal_FinalizeWithdrawal_Test [FAIL. Reason: EvmError: Revert Counterexample: calldata=0x2147ae39000000000000000000000000b4c79dab8f259c7aee6e5b2aa729821864227e8400000000000000000000000000000000000000000000000000000000000080c500000000000000000000000000000000000000000000000000000000000004cc00000000000000000000000000000000000000000000000000000000000001ca00000000000000000000000000000000000000000000000000000000000000a0000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000bcfceff2, args=[0xb4c79dab8f259c7aee6e5b2aa729821864227e84, 0x00000000000000000000000000000000000080c5, 1228, 458, 0x00000000000000000000000000000000000000000000000000000000bcfceff2]] test_finalizeWithdrawalTransaction_differential(address,address,uint256,uint256,bytes) (runs: 29, μ: 264541, ~: 265729) 

thinking

Investigating....

@smartcontracts
Copy link
Contributor Author

Same bug as before. Opening a PR to make OptimismPortal not payable.

@mergify
Copy link
Contributor

mergify bot commented Sep 15, 2022

Hey @smartcontracts! This PR has merge conflicts. Please fix them before continuing review.

@mergify mergify bot added the conflict label Sep 15, 2022
@mergify mergify bot removed the conflict label Sep 15, 2022
@mergify
Copy link
Contributor

mergify bot commented Sep 16, 2022

This PR has been added to the merge queue, and will be merged soon.

@mergify
Copy link
Contributor

mergify bot commented Sep 16, 2022

This PR is next in line to be merged, and will be merged as soon as checks pass.

@mergify mergify bot removed the on-merge-train label Sep 16, 2022
@mergify
Copy link
Contributor

mergify bot commented Sep 16, 2022

Merge failed. Please see automated check logs for more details.

Simplifies XDM initialization by moving all input variables into the
constructor. This was the best I could do, we still need an initializer
because of the stuff that the base contract inherits. The primary
benefit of this change is that it means we never have to put any
parameters into the initializer, which means we can be very certain
about parameters before we perform the upgrade.
@mergify
Copy link
Contributor

mergify bot commented Sep 16, 2022

This PR has been added to the merge queue, and will be merged soon.

@mergify mergify bot merged commit 8790156 into develop Sep 16, 2022
@mergify mergify bot deleted the sc/ctb-simplify-init branch September 16, 2022 16:44
@mergify
Copy link
Contributor

mergify bot commented Sep 16, 2022

This PR is next in line to be merged, and will be merged as soon as checks pass.

@mergify mergify bot removed the on-merge-train label Sep 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-pkg-contracts-bedrock Area: packages/contracts-bedrock

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants